-
-
Notifications
You must be signed in to change notification settings - Fork 699
[17.0][ADD] mail_activity_default_assignee #1509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 17.0
Are you sure you want to change the base?
[17.0][ADD] mail_activity_default_assignee #1509
Conversation
03791fc to
2a07a82
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
ivantodorovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add unit tests?
| "ir.model.fields", | ||
| domain=f""" | ||
| [ | ||
| ("name", "not in", {str(MAGIC_FIELDS)}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the magic fields are discarded by the ttype leaf, with exception of create_uid and write_uid.
So, I'm wondering, why don't we allow using these for the activity default user field?
| ("name", "not in", {str(MAGIC_FIELDS)}), |
| [ | ||
| ("name", "not in", {str(MAGIC_FIELDS)}), | ||
| ("ttype", "in", ["one2many","many2one", "many2many"]), | ||
| ("model_id", "=", model_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ("model_id", "=", model_id), | |
| ("model_id.model", "=", res_model), |
with this change I think we no longer need the model_id field, and so you could drop it to keep it simpler
| self.default_user_field_id = self.default_user_field_id.filtered( | ||
| lambda field: field.model_id.model == self.res_model | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid reassigning value for no reason
| self.default_user_field_id = self.default_user_field_id.filtered( | |
| lambda field: field.model_id.model == self.res_model | |
| ) | |
| if self.default_user_field_id.model_id.model != self.res_model: | |
| self.default_user_field_id = False |
| res_model = self.env[scheduler.res_model] | ||
| user_ids = scheduler._get_user_ids(res_model, fname) | ||
| if not user_ids: | ||
| continue | ||
| scheduler.activity_user_id = user_ids[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can simplify this bit by reusing some internal methods.
You can drop the _get_user_ids, too..
| res_model = self.env[scheduler.res_model] | |
| user_ids = scheduler._get_user_ids(res_model, fname) | |
| if not user_ids: | |
| continue | |
| scheduler.activity_user_id = user_ids[0] | |
| if users := scheduler._get_applied_on_records().mapped(fname): | |
| scheduler.activity_user_id = users[:1] |
ℹ️ requirement: add @api.depends('res_ids')
|
|
||
| def _get_user_ids(self, model, fname): | ||
| context = self.env.context | ||
| active_id = context.get("active_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous proposal about dropping this in favor of calling _get_applied_on_records, which is based on the record res_ids field
2a07a82 to
f36b067
Compare
|
Hello @ivantodorovich, thanks for your review. I updated, could you take a look please? |
| if not rec.res_model: | ||
| rec.model_id = False | ||
| continue | ||
| rec.model_id = self.env["ir.model"]._get_id(rec.res_model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left-over code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ja, thanks, I updated
f36b067 to
250aa06
Compare
ivantodorovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
|
||
| from odoo import api, fields, models | ||
|
|
||
| MAGIC_FIELDS = models.MAGIC_COLUMNS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
| domain="[('ttype','in', ['one2many', 'many2one', 'many2many'])," | ||
| "('model','=',res_model), ('relation', '=', 'res.users')]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add this domain in the view since it's evaluated according to the res_model field value, and add a constraint instead to check whether default_user_field_id is a feasible field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @SilvioC2C, it's actually a good idea, we can get rid of onchange
250aa06 to
5b6f429
Compare
5b6f429 to
2e60b15
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
|
Hello @hbrunn, my apologies for pinging you directly. Could you please reopen this PR and apply the "no stale" label? 🙏 |
|
Hello @SilvioC2C can you pls retake a look at this PR after it was updated? |
|
Hello @pedrobaeza, my apologies for pinging you directly. |
No description provided.